Execution API Electra: requests as a sidecar #14492
Conversation
proto/engine/v1/electra.proto
Outdated
| } | ||
|
|
||
| // ExecutionEnvelopeElectra is a container that builds on Payload V4 and includes execution requests sidecar needed post Electra | ||
| message ExecutionEnvelopeElectra { |
There was a problem hiding this comment.
should I call it something else?
There was a problem hiding this comment.
conflicts with epbs, i should rename
There was a problem hiding this comment.
I think naming it mentioning electra is self explanatory to me like we have simple ExecutionPayloadEnvelope so in my views its ok to keep this naming
There was a problem hiding this comment.
I need to change this, talked to potuz
james-prysm
changed the title
james-prysm
requested review from
potuz and
terencechain
and removed request for
nalepae
| err := s.rpcClient.CallContext(ctx, result, NewPayloadMethodV3, payloadPb, versionedHashes, parentBlockRoot) | ||
| if err != nil { | ||
| return nil, handleRPCError(err) | ||
| if executionRequests == nil { |
There was a problem hiding this comment.
This feels prone to bugs and hard to test against . Checking payload's version may be better
There was a problem hiding this comment.
yeah , the caller checks the version on the block, but the payload doesn't include the version , I can pass the block version into this function if that makes it safer
| TargetPubkey: bytesutil.PadTo([]byte{byte('h')}, fieldparams.BLSPubkeyLength), | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Something feels off about having this structure repeated across multiple tests, as well as in the fixture itself.
There was a problem hiding this comment.
do i not need to do it this way? ( assumed the payload processing worked in this way)
| @@ -14,7 +14,8 @@ type GetPayloadResponse struct { | |||
| BlobsBundle *pb.BlobsBundle | |||
| OverrideBuilder bool | |||
| // todo: should we convert this to Gwei up front? | |||
| Bid primitives.Wei | |||
| Bid primitives.Wei | |||
There was a problem hiding this comment.
Why are we so stupid of not moving to Gwei here?
What type of PR is this?
Feature
What does this PR do? Why is it needed?
implements ethereum/execution-apis#591, ethereum/execution-apis#597
engine_newPayloadV4andengine_getPayloadV4related prs Move
execution_requestsafterparent_beacon_block_rootethereum/consensus-specs#3967, Pass execution requests to Engine API as a list of bytes ethereum/consensus-specs#3969Which issues(s) does this PR fix?
Fixes #
Other notes for review
Acknowledgements